Fix buffer overflow #3
Conversation
Signed-off-by: Yves Bastide <[email protected]>
Signed-off-by: Yves Bastide <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR addresses a buffer overflow vulnerability in the MaybeEscapePattern method and improves memory management by replacing heap allocations with ArrayPool<byte>.
Key Changes:
- Fixed buffer overflow caused by incorrect index manipulation in the percent-encoding capitalization logic
- Replaced
new byte[]allocations withArrayPool<byte>.Shared.Rent()for better memory efficiency - Updated method signature to return the rented array via an
outparameter for proper cleanup
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| RobotsTxt/RobotsTxtParser.cs | Fixed buffer overflow in percent-encoding logic and replaced direct byte array allocation with ArrayPool |
| TestRobotsTxt/TestRobotsTxtParser.cs | Updated test to handle new ArrayPool-based method signature and added cleanup logic |
| TestRobotsTxt/GoogleTests.cs | Updated test to handle new ArrayPool-based method signature, added cleanup logic, and added test case for the buffer overflow scenario |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Yves Bastide <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| return dst; | ||
| return dst.AsSpan(0, j); |
There was a problem hiding this comment.
Returning a span over a rented array that will be returned to the pool creates a potential use-after-free scenario. The caller receives a span pointing to memory that may be reused once ArrayPool<byte>.Shared.Return(dst) is called. Consider either returning the array itself with the length, or copying the data to a new array before returning.
| return dst.AsSpan(0, j); | |
| var result = new byte[j]; | |
| System.Array.Copy(dst, 0, result, 0, j); | |
| return result.AsSpan(); |
| { | ||
| var actual = | ||
| Encoding.ASCII.GetString(RobotsTxtParser.MaybeEscapePattern(Encoding.UTF8.GetBytes(url)).ToArray()); | ||
| Encoding.ASCII.GetString(RobotsTxtParser.MaybeEscapePattern(Encoding.UTF8.GetBytes(url), out var dst).ToArray()); |
There was a problem hiding this comment.
Calling .ToArray() on the span creates a copy before the rented buffer is returned to the pool, but this pattern is fragile. If the .ToArray() call were removed or reordered, it would read from freed memory. Consider restructuring to make the dependency on copying before return more explicit.
Signed-off-by: Yves Bastide <[email protected]>
Fix buffer overflow in MaybeEscapePattern (
%[a-f]at pattern's end). Also replacenew byte[]withArrayPool<byte>.